Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(iroh-net): Add helper fn to enable n0 discovery publishing and resolving #2775

Merged
merged 16 commits into from
Oct 18, 2024
Merged

feat(iroh-net): Add helper fn to enable n0 discovery publishing and resolving #2775

merged 16 commits into from
Oct 18, 2024

Conversation

rklaehn
Copy link
Contributor

@rklaehn rklaehn commented Oct 2, 2024 •
edited
Loading

Description

Add a helper fn to configure n0 default discovery without having to resort to having to combine a custom discovery service.

let secret_key = SecretKey::generate();
let discovery = ConcurrentDiscovery::from_services(vec![
    Box::new(PkarrPublisher::n0_dns(secret_key.clone())),
    Box::new(DnsDiscovery::n0_dns()),
    Box::new(LocalSwarmDiscovery::new(secret_key.public())?),
]);
let ep = iroh_net::Endpoint::builder()
    .secret_key(secret_key)
    .discovery(Box::new(discovery))
    .alpns(vec![EXAMPLE_ALPN.to_vec()])
    .bind()
     .await

becomes

let ep = iroh_net::Endpoint::builder()
    .discovery_n0() // still have to do this to enable discovery!
    .alpns(vec![EXAMPLE_ALPN.to_vec()])
    .bind()
     .await

Breaking Changes

None

Notes & open questions

Should this be on or off by default?
Does anybody hate the name?
Should we also add fns for "just publish" or "just resolve" that I frequently need?

Change checklist

  • Self-review.
  • Documentation updates following the style guide, if relevant.
  • Tests if relevant.
  • All breaking changes documented.

@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 2, 2024 •
edited
Loading

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2775/docs/iroh/

Last updated: 2024-10-18T07:42:28Z

@rklaehn rklaehn requested a review from flub October 2, 2024 12:48
@rklaehn rklaehn mentioned this pull request Oct 2, 2024
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 2, 2024 •
edited
Loading

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: 7b6b582

@rklaehn rklaehn force-pushed the discovery-builder-shortcuts branch from efbac73 to b09c11b Compare October 2, 2024 15:18
flub
flub reviewed Oct 16, 2024
View reviewed changes
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a nice improvement! Even if the exactly locations of things may change before 1.0. Any reason this is still in draft?

iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
iroh-net/src/endpoint.rs Show resolved Hide resolved
iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
iroh-net/src/endpoint.rs Show resolved Hide resolved
iroh-net/src/endpoint.rs Show resolved Hide resolved
iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
iroh-net/src/endpoint.rs Show resolved Hide resolved
iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
@rklaehn
Copy link
Contributor Author

rklaehn commented Oct 17, 2024

Looks like a nice improvement! Even if the exactly locations of things may change before 1.0. Any reason this is still in draft?

I don't think it is due to code quality, just that we had a long discussion what we want to do about discovery.

@rklaehn rklaehn marked this pull request as ready for review October 17, 2024 08:01
@rklaehn rklaehn requested a review from flub October 17, 2024 09:42
flub
flub reviewed Oct 17, 2024
View reviewed changes
Copy link
Contributor

@flub flub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is looking fine, but would love to also have @ramfox 's opinion on it.

iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
iroh-net/src/endpoint.rs Outdated Show resolved Hide resolved
@flub flub requested a review from ramfox October 17, 2024 12:31
ramfox
ramfox approved these changes Oct 17, 2024
View reviewed changes
Copy link
Contributor

@ramfox ramfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. Question though: do we really need both Builder::discovery and Builder::add_discovery now?

In terms of the RFC and discovery discussions we have had / will have I think this should go in regardless.

rklaehn and others added 3 commits October 17, 2024 21:04
@rklaehn @flub
PR review - fmt
7af0c91 
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
@rklaehn @flub
PR review, shorten links
0a02e73 
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
@rklaehn @flub
PR review
817901e 
Co-authored-by: Floris Bruynooghe <flub@n0.computer>
@rklaehn
Copy link
Contributor Author

rklaehn commented Oct 17, 2024

This looks good. Question though: do we really need both Builder::discovery and Builder::add_discovery now?

In terms of the RFC and discovery discussions we have had / will have I think this should go in regardless.

Yeah, I kept Builder::discovery just in order to not break compat. I guess we can deprecate it, but maybe in a subsequent PR.

@rklaehn rklaehn added this pull request to the merge queue Oct 18, 2024
Merged via the queue into main with commit ed903ae Oct 18, 2024
27 checks passed
@rklaehn rklaehn deleted the discovery-builder-shortcuts branch November 20, 2024 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flub flub flub left review comments

@ramfox ramfox ramfox approved these changes

Assignees
No one assigned
Labels
None yet
Projects
iroh
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rklaehn @flub @ramfox